Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Differential Testing for Merkle.rs #524

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Haxry
Copy link

@Haxry Haxry commented Feb 5, 2025

Resolves #502

PR Checklist

  • Tests
  • Documentation
  • Changelog

Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for contracts-stylus canceled.

Name Link
🔨 Latest commit 861b0f3
🔍 Latest deploy log https://app.netlify.com/sites/contracts-stylus/deploys/67a35f709be3d00008ded20d

@Haxry Haxry marked this pull request as draft February 5, 2025 12:55
@Haxry
Copy link
Author

Haxry commented Feb 5, 2025

@0xNeshi i have added one differential test .
I have used merkle-tree-rs instead of rs-merkle because rs-merkle only supports sha256 and sha384, to use it for testing i have to implement shabuilder just like keccakbuilder.

Please let me know for any changes required !

@@ -661,4 +663,36 @@ mod tests {
);
assert!(verification.is_err());
}

#[test]
fn test_differential_verifies_valid_proof() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make this a differential fuzz test. That way we can test outputs for a huge number of inputs, and not just hardcoded ones.
Ideally we would configure the generated input to fit the expected data for a merkle proof validation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xNeshi i'll make the changes asap

Comment on lines +692 to +696
assert_eq!(
rebuilt_root, root,
"Rebuilt root does not match expected root"
);
assert!(verification, "Verifier::verify returned false");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the result of Verifier::verify should be true only if rebuilt_root == root (i.e. the merkle proof is valid, so process_proof generated the correct root), then the assertion should be:

assert_eq!(verification == (rebuilt_root == root), "Mismatch found");

@@ -14,6 +14,8 @@ num-traits.workspace = true
zeroize.workspace = true
educe.workspace = true
hex-literal.workspace = true
ethers-core = "1.0.2"
merkle-tree-rs = "0.1.0"
Copy link
Collaborator

@0xNeshi 0xNeshi Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have used merkle-tree-rs instead of rs-merkle because rs-merkle only supports sha256 and sha384, to use it for testing i have to implement shabuilder just like keccakbuilder.

I'd prefer we use rs-merkle, as it's more popular and thus more reliable as a litmus test.
We should be able to easily configure it to use keccak256.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Add Differential Tests for Functions in _merkle.rs_
2 participants